Extend BatteryMeter display#1967
Conversation
BenBE
added
enhancement
|
.oO(Nice, quite a stunt, to cover all of the BSD's with this. :) |
BenBE
force-pushed
the
battery-extension
branch
from
1dc77c5 to
4d3bece
Compare
BenBE
force-pushed
the
battery-extension
branch
2 times, most recently
from
98680b3 to
422d5f3
Compare
📝 WalkthroughSummaryThis PR refactors the battery reporting infrastructure across all supported platforms (Linux, macOS, FreeBSD, OpenBSD, NetBSD, DragonflyBSD, Solaris, PCP) to provide extended battery metrics: current power draw, remaining/full energy capacity, and (dis)charge time estimates. Core ChangesA new
Implementation QualityBatteryMeter.c (+127/-19):
Platform implementations (+456/-77):
Strengths: Consistent API reduces complexity and parameter confusion; sensible defaults for missing metrics; comprehensive cross-platform coverage; power sign convention preserved for compact mode efficiency. Commit StructureSingle monolithic commit combining all platform changes. While this ensures atomicity and avoids incomplete state, the scope is large (22 file changes, +583/-96 lines) with platform-specific logic tightly integrated. Future maintenance or selective backports would benefit from per-platform commit separation. Code review effort: High across platform implementations (new ACPI/sysfs parsing, energy aggregation); Medium for header declarations. WalkthroughThis PR consolidates battery outputs into a BatteryInfo struct (ac, percent, powerCurr W, energyCurr Wh, energyFull Wh). BatteryMeter now calls Platform_getBattery(&info), validates percent, detects haveEnergy/havePower, classifies charging/discharging/stable, computes whole-minute remaining/to-full estimates, and formats both TEXT_METERMODE and compact displays. Platform implementations across Darwin, FreeBSD, DragonFlyBSD, Linux, NetBSD, OpenBSD, PCP, Solaris, and Unsupported were updated to populate BatteryInfo; PCP also adds Denki metrics. Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)darwin/Platform.cdarwin/Platform.c:9:10: fatal error: 'config.h' file not found ... [truncated 694 characters] ... "/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" BatteryMeter.cBatteryMeter.c:10:10: fatal error: 'config.h' file not found ... [truncated 693 characters] ... "/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" dragonflybsd/Platform.cdragonflybsd/Platform.c:9:10: fatal error: 'config.h' file not found ... [truncated 706 characters] ... infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include"
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbsd/Platform.c (1)
471-485:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelease the proplib objects on every exit path.
prop_dictionary_recv_ioctl()returns a dictionary owned by the caller,prop_dictionary_iterator()andprop_array_iterator()create iterator objects that must be released explicitly, and iterators retain their underlying collections. Currently,dict,devIter, and eachfieldsIterare never released, causing a memory leak on every call to this function. With repeated battery polling, this accumulates over time.All three objects must be released:
dictviaprop_object_release()(owned by caller fromprop_dictionary_recv_ioctl())devIterviaprop_object_iterator_release()(created byprop_dictionary_iterator())fieldsIterviaprop_object_iterator_release()(created byprop_array_iterator())Possible fix
void Platform_getBattery(BatteryInfo* info) { - prop_dictionary_t dict, fields, props; + prop_dictionary_t dict = NULL, fields, props; prop_object_t device, class; + prop_object_iterator_t devIter = NULL; + prop_object_iterator_t fieldsIter = NULL; @@ - prop_object_iterator_t devIter = prop_dictionary_iterator(dict); + devIter = prop_dictionary_iterator(dict); if (devIter == NULL) goto error; @@ - prop_object_iterator_t fieldsIter = prop_array_iterator(fieldsArray); + fieldsIter = prop_array_iterator(fieldsArray); if (fieldsIter == NULL) goto error; @@ while ((fields = prop_object_iterator_next(fieldsIter)) != NULL) { ... } + + prop_object_iterator_release(fieldsIter); + fieldsIter = NULL; } + + prop_object_iterator_release(devIter); + devIter = NULL; + prop_object_release(dict); + dict = NULL; error: + if (fieldsIter != NULL) + prop_object_iterator_release(fieldsIter); + if (devIter != NULL) + prop_object_iterator_release(devIter); + if (dict != NULL) + prop_object_release(dict); if (fd != -1) close(fd); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbsd/Platform.c` around lines 471 - 485, The code leaks proplib objects: the dictionary returned by prop_dictionary_recv_ioctl (dict) and the iterators created by prop_dictionary_iterator (devIter) and prop_array_iterator (fieldsIter) must be released on every exit path; update the function so that before jumping to the error/exit label or returning you call prop_object_release(dict) when dict is non-NULL and prop_object_iterator_release(devIter) and prop_object_iterator_release(fieldsIter) when those iterators are non-NULL (also release any fieldsIter created inside the loop before continuing), ensuring you don't release objects twice and that fieldsArray/device handling remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BatteryMeter.c`:
- Around line 116-141: The compact-mode token concatenation happens because the
AC/bat labels written by xSnprintf (the calls that print "%s" with "AC"/"AC+bat"
and the "bat" literal) lack a trailing separator; change those format strings to
include a space (e.g., "%s " and "bat ") so tokens don't get glued, and when
printing power in the xSnprintf call that uses info.powerCurr (inside the
isCharging || isDischarging branch) normalize the sign by printing the absolute
value (use fabs(info.powerCurr) or equivalent) so discharging shows positive
watts consistent with text mode; update the xSnprintf invocations that append to
buf/len accordingly.
In `@darwin/Platform.c`:
- Around line 734-761: The code is incorrectly assigning raw percentage values
(cap_current/cap_max) into Wh fields info->energyCurr and info->energyFull;
remove the two assignments so only info->percent = 100.0 * cap_current / cap_max
is kept and leave info->energyCurr and info->energyFull as NaN (do not populate
them from cap_current/cap_max). Update the block that checks cap_max > 0.0 (the
lines that set info->energyCurr = cap_current; and info->energyFull = cap_max;)
to remove those assignments and keep only the percent calculation.
In `@linux/Platform.c`:
- Around line 1095-1104: Reverse the probe order so sysfs is tried before
procfs: when Platform_Battery_method is BAT_SYS call
Platform_Battery_getSysData(&Platform_Battery_cache) first and if
isNonnegative(Platform_Battery_cache.percent) leave method as BAT_SYS; if that
fails set Platform_Battery_method = BAT_PROC and call
Platform_Battery_getProcData(&Platform_Battery_cache) as a fallback and only
then set Platform_Battery_method = BAT_ERR if percent is still not nonnegative.
Use the existing symbols Platform_Battery_method, Platform_Battery_getSysData,
Platform_Battery_getProcData, Platform_Battery_cache, BAT_SYS, BAT_PROC, BAT_ERR
and isNonnegative to implement this change.
- Around line 841-844: Platform_Battery_getProcData currently replaces a valid
procfs percent with NAN whenever procAcpiCheck() fails; change it to always read
the procfs battery percentage and only set percent to NAN if
Platform_Battery_getProcBatInfo() itself indicates failure. Concretely, call
Platform_Battery_getProcBatInfo() unconditionally to populate info->percent,
assign info->ac = procAcpiCheck() but leave info->ac as AC_ERROR if adapter
detection failed, and do not overwrite a valid percent with NAN based solely on
procAcpiCheck() failing; only set percent to NAN when
Platform_Battery_getProcBatInfo() reports an error.
In `@openbsd/Platform.c`:
- Around line 391-457: The code only calls findDevice("acpibat0", ...) which
collects battery metrics from a single pack; change the logic to iterate over
all acpibat devices (e.g., for i = 0; findDevice(name, mib, &snsrdev, &sdlen);
++i) using a formatted name like "acpibat%d" and accumulate totalFull,
totalRemain and totalPower per-device (the blocks that read SENSOR_WATTHOUR,
SENSOR_INTEGER, SENSOR_WATTS and update batteryFull, batteryRemain,
batteryState, batteryPower) into the existing totals; keep the final
percent/energy/power calculations using the aggregated totals (referencing
findDevice, totalFull, totalRemain, totalPower, and the sysctl queries for
SENSOR_WATTHOUR/SENSOR_WATTS/SENSOR_INTEGER).
In `@pcp/Platform.c`:
- Around line 880-883: The AC state logic is inverted: instead of setting
info->ac = AC_PRESENT when count < 1, set AC_PRESENT when there is at least one
battery (count >= 1) and then override to AC_ABSENT if any battery is
discharging (power < 0). Update the block that currently checks count and
assigns info->ac so the flow is: if count < 1 set a fallback/ERROR state (or
return appropriately), otherwise set info->ac = AC_PRESENT, iterate battery
instances to check power and if any power < 0 set info->ac = AC_ABSENT; apply
the same fix to the analogous block around the second occurrence (the block
referenced at lines ~912-914). Use the existing symbols info->ac, AC_PRESENT,
AC_ABSENT, count and the battery power checks to locate and change the logic.
- Around line 892-893: The code uses batteryEnergyFull[i].d directly as the
CLAMP upper bound which can be negative and cause info->energyCurr to go
negative; compute a non-negative full value first (e.g., double full =
isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d : 0) and then use
CLAMP(batteryEnergyCurr[i].d, 0, full) to update info->energyCurr and add full
(not the raw batteryEnergyFull) to info->energyFull so both updates guard
against negative full-capacity samples (references: info->energyCurr,
info->energyFull, batteryEnergyCurr, batteryEnergyFull, CLAMP, isNonnegative).
---
Outside diff comments:
In `@netbsd/Platform.c`:
- Around line 471-485: The code leaks proplib objects: the dictionary returned
by prop_dictionary_recv_ioctl (dict) and the iterators created by
prop_dictionary_iterator (devIter) and prop_array_iterator (fieldsIter) must be
released on every exit path; update the function so that before jumping to the
error/exit label or returning you call prop_object_release(dict) when dict is
non-NULL and prop_object_iterator_release(devIter) and
prop_object_iterator_release(fieldsIter) when those iterators are non-NULL (also
release any fieldsIter created inside the loop before continuing), ensuring you
don't release objects twice and that fieldsArray/device handling remains
unchanged.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02679205-318c-40a3-9203-a042a4323268
📒 Files selected for processing (21)
BatteryMeter.cBatteryMeter.hdarwin/Platform.cdarwin/Platform.hdragonflybsd/Platform.cdragonflybsd/Platform.hfreebsd/Platform.cfreebsd/Platform.hlinux/Platform.clinux/Platform.hnetbsd/Platform.cnetbsd/Platform.hopenbsd/Platform.copenbsd/Platform.hpcp/Metric.hpcp/Platform.cpcp/Platform.hsolaris/Platform.csolaris/Platform.hunsupported/Platform.cunsupported/Platform.h
| if (Platform_Battery_method == BAT_PROC) { | ||
| Platform_Battery_getProcData(percent, isOnAC); | ||
| if (!isNonnegative(*percent)) | ||
| Platform_Battery_getProcData(&Platform_Battery_cache); | ||
| if (!isNonnegative(Platform_Battery_cache.percent)) | ||
| Platform_Battery_method = BAT_SYS; | ||
| } | ||
| if (Platform_Battery_method == BAT_SYS) { | ||
| Platform_Battery_getSysData(percent, isOnAC); | ||
| if (!isNonnegative(*percent)) | ||
| Platform_Battery_getSysData(&Platform_Battery_cache); | ||
| if (!isNonnegative(Platform_Battery_cache.percent)) | ||
| Platform_Battery_method = BAT_ERR; | ||
| } |
There was a problem hiding this comment.
Prefer sysfs before procfs for the new battery telemetry.
Platform_Battery_getProcData() never fills powerCurr or energy*, so the current BAT_PROC-first flow leaves the new rate/capacity/time-estimate inputs permanently NAN on hosts that expose both procfs and sysfs. Sysfs needs to be the preferred source now, with procfs kept as a fallback for older setups.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@linux/Platform.c` around lines 1095 - 1104, Reverse the probe order so sysfs
is tried before procfs: when Platform_Battery_method is BAT_SYS call
Platform_Battery_getSysData(&Platform_Battery_cache) first and if
isNonnegative(Platform_Battery_cache.percent) leave method as BAT_SYS; if that
fails set Platform_Battery_method = BAT_PROC and call
Platform_Battery_getProcData(&Platform_Battery_cache) as a fallback and only
then set Platform_Battery_method = BAT_ERR if percent is still not nonnegative.
Use the existing symbols Platform_Battery_method, Platform_Battery_getSysData,
Platform_Battery_getProcData, Platform_Battery_cache, BAT_SYS, BAT_PROC, BAT_ERR
and isNonnegative to implement this change.
| bool found = findDevice("acpibat0", mib, &snsrdev, &sdlen); | ||
|
|
||
| *percent = NAN; | ||
| if (found) { | ||
| bool haveTotalFull = false; | ||
| bool haveTotalRemain = false; | ||
| bool haveTotalPower = false; | ||
|
|
||
| int64_t totalFull = 0; | ||
| int64_t totalRemain = 0; | ||
| int64_t totalPower = 0; | ||
|
|
||
| /* See "sys/dev/acpi/acpibat.c" of OpenBSD source code for the indices | ||
| of the last field. */ | ||
| mib[3] = SENSOR_WATTHOUR; | ||
| mib[4] = 0; /* "last full capacity" */ | ||
| double last_full_capacity = 0; | ||
| bool haveBatteryFull = false; | ||
| int64_t batteryFull = 0; | ||
| if (sysctl(mib, 5, &s, &slen, NULL, 0) != -1) | ||
| last_full_capacity = s.value; | ||
| if (last_full_capacity > 0) { | ||
| batteryFull = s.value; | ||
|
|
||
| if (batteryFull > 0) | ||
| haveBatteryFull = true; | ||
|
|
||
| if (haveBatteryFull) { | ||
| mib[3] = SENSOR_WATTHOUR; | ||
| mib[4] = 3; /* "remaining capacity" */ | ||
| if (sysctl(mib, 5, &s, &slen, NULL, 0) != -1) { | ||
| double charge = s.value; | ||
| *percent = 100 * (charge / last_full_capacity); | ||
| if (charge >= last_full_capacity) { | ||
| *percent = 100; | ||
| int64_t batteryRemain = s.value; | ||
| if (batteryRemain >= 0) { | ||
| totalRemain += batteryRemain; | ||
| totalFull += batteryFull; | ||
| haveTotalRemain = true; | ||
| haveTotalFull = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (haveTotalRemain && haveTotalFull && totalFull > 0) { | ||
| info->percent = ((double) totalRemain * 100.0) / (double) totalFull; | ||
| if (totalRemain >= totalFull) | ||
| info->percent = 100; | ||
|
|
||
| info->energyCurr = (double) totalRemain / 1000000.0; | ||
| info->energyFull = (double) totalFull / 1000000.0; | ||
| } | ||
|
|
||
| mib[3] = SENSOR_INTEGER; | ||
| mib[4] = 0; /* "battery state" */ | ||
| int64_t batteryState = 0; | ||
| if (sysctl(mib, 5, &s, &slen, NULL, 0) != -1) | ||
| batteryState = s.value; | ||
|
|
||
| mib[3] = SENSOR_WATTS; | ||
| mib[4] = 0; /* "rate" */ | ||
| if (sysctl(mib, 5, &s, &slen, NULL, 0) != -1) { | ||
| int64_t batteryPower = s.value; | ||
| if (batteryState & 0x01) | ||
| batteryPower = -batteryPower; | ||
|
|
||
| totalPower += batteryPower; | ||
| haveTotalPower = true; | ||
| } | ||
|
|
||
| if (haveTotalPower) { | ||
| info->powerCurr = (double) totalPower / 1000000.0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't hardcode a single battery device.
Line 391 only queries acpibat0, so totalFull, totalRemain, and totalPower can never include any other battery device even though this code now derives aggregate battery totals from them. On multi-battery hardware, the reported percent, Wh, and time estimates will reflect only the first pack.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openbsd/Platform.c` around lines 391 - 457, The code only calls
findDevice("acpibat0", ...) which collects battery metrics from a single pack;
change the logic to iterate over all acpibat devices (e.g., for i = 0;
findDevice(name, mib, &snsrdev, &sdlen); ++i) using a formatted name like
"acpibat%d" and accumulate totalFull, totalRemain and totalPower per-device (the
blocks that read SENSOR_WATTHOUR, SENSOR_INTEGER, SENSOR_WATTS and update
batteryFull, batteryRemain, batteryState, batteryPower) into the existing
totals; keep the final percent/energy/power calculations using the aggregated
totals (referencing findDevice, totalFull, totalRemain, totalPower, and the
sysctl queries for SENSOR_WATTHOUR/SENSOR_WATTS/SENSOR_INTEGER).
There was a problem hiding this comment.
Can you point to documentation where it describes how to best find the number of available devices and how they are named?
There was a problem hiding this comment.
@BenBE — here are the relevant references:
Official documentation:
acpibat(4)— describes the driver; battery status is accessible viasysctl(8)orsensorsd(8).sysctl(2)— documents theHW_SENSORSMIB layout: the third level is an array ofstruct sensordevstructures for each sensor device, and the third/fourth/fifth levels together address individualstruct sensorreadings. The third level is the numeric device index whosexnameis matched against the device name.
The enumeration protocol (already implemented in your findDevice()) is: iterate mib[2] from 0; ENXIO means no device at this slot (skip); ENOENT means no more devices (stop). Batteries are named acpibat0, acpibat1, etc.
Real-world confirmation:
A two-battery system exposes hw.sensors.acpibat0.* and hw.sensors.acpibat1.* as sibling sensordev entries with identical sensor indices.
Prior-art for the iteration pattern:
The distatus/battery library on OpenBSD does exactly this: it loops over consecutive MIB indices, calls sysctl for each sensordev, breaks on ENOENT, and checks whether xname has the prefix "acpibat".
Suggested adaptation for Platform_getBattery: instead of calling findDevice("acpibat0", ...) once, inline the sensordev scan and match any xname with strncmp(snsrdev.xname, "acpibat", 7) == 0 (or equivalent), accumulating totalFull/totalRemain/totalPower across all matching devices before computing the final percent/energy/power fields.
| info->energyCurr += CLAMP(batteryEnergyCurr[i].d, 0, batteryEnergyFull[i].d); | ||
| info->energyFull += isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d : 0; |
There was a problem hiding this comment.
Guard energyCurr from going negative on invalid full-capacity samples.
Line 892 uses batteryEnergyFull[i].d directly as the clamp upper bound. If that value is negative, energyCurr can become negative. Clamp against a non-negative full first.
Proposed patch
info->energyCurr = 0.0;
info->energyFull = 0.0;
for (i = 0; i < count; i++) {
- info->energyCurr += CLAMP(batteryEnergyCurr[i].d, 0, batteryEnergyFull[i].d);
- info->energyFull += isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d : 0;
+ double full = isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d : 0.0;
+ info->energyCurr += CLAMP(batteryEnergyCurr[i].d, 0.0, full);
+ info->energyFull += full;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pcp/Platform.c` around lines 892 - 893, The code uses batteryEnergyFull[i].d
directly as the CLAMP upper bound which can be negative and cause
info->energyCurr to go negative; compute a non-negative full value first (e.g.,
double full = isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d :
0) and then use CLAMP(batteryEnergyCurr[i].d, 0, full) to update
info->energyCurr and add full (not the raw batteryEnergyFull) to
info->energyFull so both updates guard against negative full-capacity samples
(references: info->energyCurr, info->energyFull, batteryEnergyCurr,
batteryEnergyFull, CLAMP, isNonnegative).
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BatteryMeter.c`:
- Around line 83-88: The block that appends an extra separator (the if (ret &&
len > 2) { *buf++ = ','; *buf++ = ' '; *buf = 0; len -= 2; }) causes a double
comma in TEXT_METERMODE output because the AC prefix written earlier already
includes ", ". Remove this entire conditional block (references: variables ret,
len, buf and the TEXT_METERMODE output path) so no additional ", " is appended;
no other changes are needed to the surrounding code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5eef6dbc-1164-41ed-9430-663fb5dacd29
📒 Files selected for processing (1)
BatteryMeter.c
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
BatteryMeter.c (1)
137-141:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize discharging watts sign in compact mode.
Line 140 prints raw
info.powerCurr, so discharging displays negative watts, while text mode (Line 93) shows positive watts for discharging. Keep both modes consistent.Suggested patch
if (isCharging || isDischarging) { ret = xSnprintf( buf, len, "%.1fW @ %.1f/%.1fWh", - info.powerCurr, info.energyCurr, info.energyFull + isDischarging ? -info.powerCurr : info.powerCurr, + info.energyCurr, info.energyFull ); buf += ret; len -= ret;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BatteryMeter.c` around lines 137 - 141, Compact-mode watt value uses raw info.powerCurr causing negative watts during discharging; update the xSnprintf call inside the if (isCharging || isDischarging) block to pass a normalized (absolute) watt value so discharging shows positive watts like text mode — e.g. replace the first format argument info.powerCurr with a normalized expression (use -info.powerCurr when isDischarging, or fabsf(info.powerCurr)) while keeping the rest of the arguments (info.energyCurr, info.energyFull) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BatteryMeter.c`:
- Around line 49-51: The percent-only fallback in BatteryMeter.c (when
haveEnergy is false) currently writes only "%.1f%%" to this->txtBuffer and
returns, losing AC/battery context; modify the early-return branch where
haveEnergy is checked to include info.ac in the formatted string (use info.ac to
choose an "AC"/"Battery" marker or a suffix/prefix) so that the call that writes
to this->txtBuffer includes both the percent (info.percent) and the AC state
(info.ac) before returning.
---
Duplicate comments:
In `@BatteryMeter.c`:
- Around line 137-141: Compact-mode watt value uses raw info.powerCurr causing
negative watts during discharging; update the xSnprintf call inside the if
(isCharging || isDischarging) block to pass a normalized (absolute) watt value
so discharging shows positive watts like text mode — e.g. replace the first
format argument info.powerCurr with a normalized expression (use -info.powerCurr
when isDischarging, or fabsf(info.powerCurr)) while keeping the rest of the
arguments (info.energyCurr, info.energyFull) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ac465c8f-a119-478f-8226-175dae17a47b
📒 Files selected for processing (1)
BatteryMeter.c
| if (!haveEnergy) { | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%.1f%%", info.percent); | ||
| return; |
There was a problem hiding this comment.
Preserve AC source in the percent-only fallback.
At Line 49-Line 51, the early return prints only %.1f%%. When energy metrics are unavailable, users lose AC/battery context even though info.ac is already known.
Suggested patch
/* Without energy data there is nothing useful to show beyond the percent. */
if (!haveEnergy) {
- xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%.1f%%", info.percent);
+ const char* src =
+ (info.ac == AC_PRESENT) ? " (AC)" :
+ (info.ac == AC_ABSENT) ? " (bat)" : "";
+ xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%.1f%%%s", info.percent, src);
return;
}🧰 Tools
🪛 ast-grep (0.42.2)
[warning] 49-49: Do not use sizeof(this) to get the number of bytes of the object in memory. It returns the size of the pointer, not the size of the object.
Context: sizeof(this->txtBuffer)
Note: [CWE-467]: Use of sizeof() on a Pointer Type [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/ARR01-C.+Do+not+apply+the+sizeof+operator+to+a+pointer+when+taking+the+size+of+an+array
(sizeof-this-c)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@BatteryMeter.c` around lines 49 - 51, The percent-only fallback in
BatteryMeter.c (when haveEnergy is false) currently writes only "%.1f%%" to
this->txtBuffer and returns, losing AC/battery context; modify the early-return
branch where haveEnergy is checked to include info.ac in the formatted string
(use info.ac to choose an "AC"/"Battery" marker or a suffix/prefix) so that the
call that writes to this->txtBuffer includes both the percent (info.percent) and
the AC state (info.ac) before returning.
In compact mode, better to include the sign (negative for discharging, positive for charging). |
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbsd/Platform.c (1)
471-485:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelease proplib objects to prevent memory leak.
dict,devIter, and eachfieldsItermust be released viaprop_object_release()when no longer needed. NetBSD proplib objects are reference-counted and require explicit release—failure to do so leaks memory on every battery refresh cycle in long-running sessions. Release on both success and error paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0a0ac52a-cfb8-45fa-b14d-6af829fe988e
📒 Files selected for processing (12)
BatteryMeter.cBatteryMeter.hdarwin/Platform.cdragonflybsd/Platform.cfreebsd/Platform.clinux/Platform.cnetbsd/Platform.copenbsd/Platform.cpcp/Metric.hpcp/Platform.csolaris/Platform.cunsupported/Platform.c
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbsd/Platform.c (1)
471-486:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResource leak: proplib objects never released.
dict,devIter, andfieldsIterare allocated but never freed. ThefieldsIteris created each loop iteration without release. Per proplib API, these requireprop_object_release()andprop_object_iterator_release().Proposed fix
void Platform_getBattery(BatteryInfo* info) { - prop_dictionary_t dict, fields, props; + prop_dictionary_t dict = NULL; + prop_dictionary_t fields, props; prop_object_t device, class; + prop_object_iterator_t devIter = NULL; // ... initialization unchanged ... if (prop_dictionary_recv_ioctl(fd, ENVSYS_GETDICTIONARY, &dict) != 0) - goto error; + goto cleanup; - prop_object_iterator_t devIter = prop_dictionary_iterator(dict); + devIter = prop_dictionary_iterator(dict); if (devIter == NULL) - goto error; + goto cleanup; while ((device = prop_object_iterator_next(devIter)) != NULL) { prop_object_t fieldsArray = prop_dictionary_get_keysym(dict, device); if (fieldsArray == NULL) - goto error; + goto cleanup; prop_object_iterator_t fieldsIter = prop_array_iterator(fieldsArray); if (fieldsIter == NULL) - goto error; + goto cleanup; // ... process fields ... + prop_object_iterator_release(fieldsIter); } -error: +cleanup: + if (devIter) + prop_object_iterator_release(devIter); + if (dict) + prop_object_release(dict); if (fd != -1) close(fd); }
♻️ Duplicate comments (2)
dragonflybsd/Platform.c (1)
479-486:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefer sysctl percent as authoritative when aggregation is incomplete.
The aggregation loop may skip battery units on ioctl failure or missing voltage data. Overwriting
info->percentwith the partial aggregate can produce an incorrect percentage compared to the kernel-reportedhw.acpi.battery.lifevalue already stored at line 382. Only setinfo->percentfrom the aggregate when the sysctl failed (i.e.,info->percentis stillNAN).Proposed fix
if (haveTotalRemain && haveTotalFull && totalFull > 0) { - info->percent = ((double) totalRemain * 100.0) / (double) totalFull; - if (totalRemain >= totalFull) - info->percent = 100; + if (isnan(info->percent)) { + info->percent = ((double) totalRemain * 100.0) / (double) totalFull; + if (totalRemain >= totalFull) + info->percent = 100.0; + } info->energyCurr = (double) totalRemain / 1000000.0; info->energyFull = (double) totalFull / 1000000.0;freebsd/Platform.c (1)
507-514:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep sysctl percent as fallback for incomplete aggregation.
Same issue as DragonFlyBSD: the loop can skip units when
ACPIIO_BATT_GET_BIXorACPIIO_BATT_GET_BSTfails. The kernel-reportedhw.acpi.battery.life(already stored at line 415) is more reliable than a partial aggregate. Only overwriteinfo->percentwhen the sysctl value is unavailable.Proposed fix
if (haveTotalRemain && haveTotalFull && totalFull > 0) { - info->percent = ((double) totalRemain * 100.0) / (double) totalFull; - if (totalRemain >= totalFull) - info->percent = 100; + if (isnan(info->percent)) { + info->percent = ((double) totalRemain * 100.0) / (double) totalFull; + if (totalRemain >= totalFull) + info->percent = 100.0; + } info->energyCurr = (double) totalRemain / 1000000.0; info->energyFull = (double) totalFull / 1000000.0;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0881be70-ec0a-435f-9e26-53a032962ece
📒 Files selected for processing (12)
BatteryMeter.cBatteryMeter.hdarwin/Platform.cdragonflybsd/Platform.cfreebsd/Platform.clinux/Platform.cnetbsd/Platform.copenbsd/Platform.cpcp/Metric.hpcp/Platform.csolaris/Platform.cunsupported/Platform.c
| if (!isNonnegative(info.percent)) { | ||
| this->values[0] = NAN; | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "N/A"); | ||
| return; | ||
| } | ||
|
|
||
| this->values[0] = percent; | ||
|
|
||
| const char* text; | ||
| switch (isOnAC) { | ||
| case AC_PRESENT: | ||
| text = this->mode == TEXT_METERMODE ? " (Running on A/C)" : "(A/C)"; | ||
| break; | ||
| case AC_ABSENT: | ||
| text = this->mode == TEXT_METERMODE ? " (Running on battery)" : "(bat)"; | ||
| break; | ||
| case AC_ERROR: | ||
| default: | ||
| text = ""; | ||
| break; | ||
| this->values[0] = info.percent; |
There was a problem hiding this comment.
Enforce the upper bound on percent here too.
The BatteryInfo contract says percent is [0..100], but this branch only rejects negative/unknown values. Anything above 100 is written straight into this->values[0] even though the meter total is fixed at 100.
🧰 Tools
🪛 ast-grep (0.42.2)
[warning] 39-39: Do not use sizeof(this) to get the number of bytes of the object in memory. It returns the size of the pointer, not the size of the object.
Context: sizeof(this->txtBuffer)
Note: [CWE-467]: Use of sizeof() on a Pointer Type [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/ARR01-C.+Do+not+apply+the+sizeof+operator+to+a+pointer+when+taking+the+size+of+an+array
(sizeof-this-c)
🪛 Cppcheck (2.20.0)
[style] 39-39: The function 'Vector_quickSort' is never used.
(unusedFunction)
| bool haveEnergy = isNonnegative(info.energyCurr) && isNonnegative(info.energyFull); | ||
|
|
||
| /* Without energy data there is nothing useful to show beyond the percent. */ | ||
| if (!haveEnergy) { | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%.1f%%", info.percent); | ||
| return; | ||
| } | ||
|
|
||
| bool havePower = isfinite(info.powerCurr); |
There was a problem hiding this comment.
Do not discard powerCurr when energy is unavailable.
This fallback reduces the output to percent-only even if rate telemetry is present. Per the supplied stack context, Darwin currently provides powerCurr, so this branch makes the new charge/discharge-rate work invisible there. Compute havePower before the early return and show a watt-based fallback when energyCurr/energyFull are missing.
🧰 Tools
🪛 ast-grep (0.42.2)
[warning] 49-49: Do not use sizeof(this) to get the number of bytes of the object in memory. It returns the size of the pointer, not the size of the object.
Context: sizeof(this->txtBuffer)
Note: [CWE-467]: Use of sizeof() on a Pointer Type [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/ARR01-C.+Do+not+apply+the+sizeof+operator+to+a+pointer+when+taking+the+size+of+an+array
(sizeof-this-c)
🪛 Cppcheck (2.20.0)
[style] 53-53: The function 'String_startsWith' is never used.
(unusedFunction)
| ACPresence ac; | ||
|
|
||
| double percent; /* [0..100], NAN if unknown */ | ||
| double powerCurr; /* instantaneous power in W, NAN if unknown */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document the powerCurr sign contract.
BatteryMeter_updateValues() derives charging vs. discharging entirely from this field's sign, but the header only documents the unit. Please state the convention here explicitly, e.g. positive while charging and negative while discharging, so platform collectors implement the same contract.
| if (!haveBatteryPower && haveBatteryCurrent && haveBatteryVoltage) { | ||
| batteryPower = (batteryCurrent * batteryVoltage) / 1000000; | ||
| haveBatteryPower = true; | ||
| } | ||
|
|
||
| if (!now && full && isNonnegative(capacityLevel)) | ||
| totalRemain += capacityLevel * fullCharge; | ||
| if (haveBatteryPower) { | ||
| if (batteryIsDischarging) | ||
| batteryPower = -batteryPower; | ||
|
|
||
| totalPower += batteryPower; | ||
| havePower = true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Linux kernel documentation and driver conventions for CURRENT_NOW signedness
rg -n -C3 "CURRENT_NOW" --type=c | head -80
# Search for any existing handling of signed current in power_supply drivers
ast-grep --pattern 'POWER_SUPPLY_PROP_CURRENT_NOW'Repository: htop-dev/htop
Length of output: 399
🏁 Script executed:
#!/bin/bash
# Get the full context of the battery power calculation
echo "=== Checking variable declarations and initialization ==="
rg -n "batteryCurrent|batteryPower|batteryIsDischarging|batteryVoltage" linux/Platform.c | head -40
echo -e "\n=== Full battery reading section ==="
sed -n '950,1050p' linux/Platform.cRepository: htop-dev/htop
Length of output: 4456
🏁 Script executed:
#!/bin/bash
# Check how val is parsed - does it handle negative numbers?
echo "=== Checking val parsing context ==="
sed -n '900,950p' linux/Platform.c
# Look for any comments about battery or current in the file
echo -e "\n=== Comments about battery/power handling ==="
rg -n "battery|CURRENT|current" linux/Platform.c | grep -i "comment\|negative\|discharge" | head -20Repository: htop-dev/htop
Length of output: 1742
🌐 Web query:
Linux kernel POWER_SUPPLY_PROP_CURRENT_NOW sysfs convention signedness negative discharge
💡 Result:
In the Linux kernel power supply class, the sysfs attribute POWER_SUPPLY_PROP_CURRENT_NOW follows a specific convention regarding signedness to distinguish between charging and discharging states [1][2]. According to the official kernel documentation, the convention is as follows: 1. Signedness: The value is treated as a signed integer [1][2]. 2. Negative Values: Negative values are used to represent battery discharging (i.e., current flowing out of the battery to power a load) [1][2]. 3. Positive Values: Positive values are used to represent battery charging (i.e., current flowing into the battery) [1][2]. This convention allows user-space applications to determine the direction of current flow for battery-backed devices by checking the sign of the integer reported in the current_now sysfs file [1][2]. For other types of power supplies, such as USB IBUS, positive values are typically used to report the current being supplied [1][2]. Drivers are responsible for converting their raw hardware readings into these standardized units (microamperes) and applying the correct sign based on the operational state [3][4].
Citations:
- 1: https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-power
- 2: https://kernel.googlesource.com/pub/scm/linux/kernel/git/rzhang/linux/+/bb48a59135926ece9b1361e8b96b33fc658830bc/Documentation/ABI/testing/sysfs-class-power
- 3: https://kernel.org/doc/html/v5.18/power/power_supply_class.html
- 4: https://unix.stackexchange.com/questions/726822/understanding-power-supply-files
🏁 Script executed:
#!/bin/bash
# Check how totalPower is used and any sign convention documentation
echo "=== Usage of totalPower after calculation ==="
sed -n '1040,1100p' linux/Platform.c
echo -e "\n=== Look for any function comments about sign convention ==="
sed -n '850,920p' linux/Platform.c | grep -A5 "^[[:space:]]*//.*power\|^[[:space:]]*/*.*power"
# Check if there's any test or usage that shows expected sign convention
echo -e "\n=== Search for power-related struct/API that shows sign expectation ==="
rg -n "totalPower|batteryPower" linux/Platform.c | tail -20Repository: htop-dev/htop
Length of output: 2333
Remove the STATUS-based negation; CURRENT_NOW is already signed per Linux kernel convention.
The Linux power supply API reports CURRENT_NOW as a signed value where negative indicates discharge and positive indicates charge. Since batteryCurrent is read directly from CURRENT_NOW (line 1034), the power calculation already has the correct sign. The subsequent negation at line 1040 based on batteryIsDischarging inverts this, causing discharge to report positive power.
Remove the sign negation or check whether batteryCurrent is already negative before applying the STATUS-based correction.
| if (Metric_desc(PCP_DENKI_ENERGY_NOW) == NULL) | ||
| return; |
There was a problem hiding this comment.
Allow partial Denki battery telemetry.
This early return drops info->powerCurr whenever denki.bat.energy_now is unavailable, even if denki.bat.power_now exists. The unified BatteryInfo contract allows unknown energy fields, and BatteryMeter can still render charge/discharge power, so this suppresses valid PCP battery data on partial Denki installations.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbsd/Platform.c (1)
471-476:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProplib iterator not released before return.
devIteris created at line 474 but never released. When jumping toerror:or reaching the end of iteration,prop_object_iterator_release(devIter)should be called. Similarly forfieldsItercreated at line 483.Proposed fix
+ prop_object_iterator_t devIter = NULL; + prop_object_iterator_t fieldsIter = NULL; + int fd = open(_PATH_SYSMON, O_RDONLY); if (fd == -1) goto error; if (prop_dictionary_recv_ioctl(fd, ENVSYS_GETDICTIONARY, &dict) != 0) goto error; - prop_object_iterator_t devIter = prop_dictionary_iterator(dict); + devIter = prop_dictionary_iterator(dict); if (devIter == NULL) goto error; while ((device = prop_object_iterator_next(devIter)) != NULL) { // ... - prop_object_iterator_t fieldsIter = prop_array_iterator(fieldsArray); + fieldsIter = prop_array_iterator(fieldsArray); if (fieldsIter == NULL) goto error; // ... loop body ... + prop_object_iterator_release(fieldsIter); + fieldsIter = NULL; } + prop_object_iterator_release(devIter); + devIter = NULL; // ... percent/power calculations ... error: + if (fieldsIter) + prop_object_iterator_release(fieldsIter); + if (devIter) + prop_object_iterator_release(devIter); + if (dict) + prop_object_release(dict); if (fd != -1) close(fd); }Also applies to: 637-640
♻️ Duplicate comments (2)
pcp/Platform.c (1)
891-894:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
energyCurrfrom invalid full-capacity samples.Line 892 uses
batteryEnergyFull[i].ddirectly as the CLAMP upper bound. If that value is negative,energyCurraccumulates an incorrect contribution while line 893 correctly excludes it fromenergyFull. Compute the sanitized full value once and use it consistently:Proposed fix
info->energyCurr = 0.0; info->energyFull = 0.0; for (i = 0; i < count; i++) { - info->energyCurr += CLAMP(batteryEnergyCurr[i].d, 0, batteryEnergyFull[i].d); - info->energyFull += isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d : 0; + double full = isNonnegative(batteryEnergyFull[i].d) ? batteryEnergyFull[i].d : 0.0; + info->energyCurr += CLAMP(batteryEnergyCurr[i].d, 0.0, full); + info->energyFull += full; }linux/Platform.c (1)
841-844:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProcfs percent discarded when AC detection fails.
Line 843 sets
info->percent = NANwhenprocAcpiCheck()returnsAC_ERROR, even though/proc/acpi/battery/*may still contain valid capacity data. The AC adapter state and battery capacity are independent data sources.Proposed fix
static void Platform_Battery_getProcData(BatteryInfo* info) { info->ac = procAcpiCheck(); - info->percent = AC_ERROR != info->ac ? Platform_Battery_getProcBatInfo() : NAN; + info->percent = Platform_Battery_getProcBatInfo(); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c9b26473-03af-404a-85f4-d4af978bc086
📒 Files selected for processing (21)
BatteryMeter.cBatteryMeter.hdarwin/Platform.cdarwin/Platform.hdragonflybsd/Platform.cdragonflybsd/Platform.hfreebsd/Platform.cfreebsd/Platform.hlinux/Platform.clinux/Platform.hnetbsd/Platform.cnetbsd/Platform.hopenbsd/Platform.copenbsd/Platform.hpcp/Metric.hpcp/Platform.cpcp/Platform.hsolaris/Platform.csolaris/Platform.hunsupported/Platform.cunsupported/Platform.h
2 participants
This is the beginning of a series of commits to extend the BatteryMeter to provide additional information like battery capacity, charge rate and (dis)charge time estimates.
For the BSD parts of this PR I'll need some input from the people on these platforms regarding the actual interface to use to get the following values:
Pointers for sample code on each of the different platforms would be nice.